fix: prevent spurious .github symlink probes in frontmatter hash GitHub API reader#45989
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
… probes in hash computation Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
✅ Test Quality Sentinel completed test quality analysis. |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #45989 does not have the 'implementation' label and has 0 new lines of code in business logic directories (threshold: 100). |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
There was a problem hiding this comment.
Pull request overview
Prevents invalid GitHub API path probes during frontmatter hash computation.
Changes:
- Filters object-form import entries.
- Skips known non-symlink path prefixes.
- Updates tests and regenerated workflows.
Show a summary per file
| File | Description |
|---|---|
actions/setup/js/frontmatter_hash_pure.cjs |
Updates import extraction and symlink probing. |
actions/setup/js/frontmatter_hash_pure.test.cjs |
Adds regression coverage. |
.github/workflows/skillet.lock.yml |
Refreshes action version annotations. |
.github/workflows/release.lock.yml |
Refreshes action version annotations. |
.github/workflows/hourly-ci-cleaner.lock.yml |
Refreshes action version annotations. |
.github/workflows/avenger.lock.yml |
Refreshes action version annotations. |
.github/workflows/agentic-auto-upgrade.yml |
Changes the weekly schedule. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 7/7 changed files
- Comments generated: 3
- Review effort level: Medium
| // Skip YAML object-form imports (uses: <path> / path: <path>) — these require full | ||
| // YAML parsing and are not supported by the text-based hash parser. The Go implementation | ||
| // silently skips them at the filesystem level (file not found); we skip them here to | ||
| // match that behavior and avoid spurious GitHub API calls. | ||
| if (item.startsWith("uses:") || item.startsWith("path:")) { | ||
| continue; |
There was a problem hiding this comment.
Fixed in 6e6a265. extractImportsFromText now extracts the path value from uses: <path> and path: <path> prefixes instead of skipping those items. The same fix was applied to the Go extractImportItem for JS/Go hash parity. All 257 workflow lock files were recompiled to reflect the updated hash values.
| if (filePath.startsWith(".github/workflows/")) { | ||
| startIndex = 3; // Skip ".github" and ".github/workflows" components | ||
| } else if (filePath.startsWith(".github/")) { | ||
| startIndex = 2; // Skip ".github" component; still check e.g. ".github/agents" |
There was a problem hiding this comment.
Fixed in 6e6a265. Added "should never probe .github or .github/workflows for a .github/workflows/ path" which records all probed paths, asserts neither .github nor .github/workflows is ever requested, and confirms .github/workflows/shared is still probed for a path like .github/workflows/shared/otlp.md.
| on: | ||
| schedule: | ||
| - cron: "21 3 * * 5" # Weekly (auto-upgrade) | ||
| - cron: "11 4 * * 6" # Weekly (auto-upgrade) |
There was a problem hiding this comment.
Fixed in 6e6a265. The schedule drift ("11 4 * * 6" → Saturday) was reverted to the original "21 3 * * 5" (Friday). This was unintended regeneration noise unrelated to the PR's changes.
🧪 Test Quality Sentinel Report✅ Test Quality Score: 100/100 — Excellent
📊 Metrics (3 tests)
Verdict
Highlights:
Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "awmgmcpg"See Network Configuration for more information.
|
There was a problem hiding this comment.
The two fixes are correct and well-tested.
Bug 1 — extractImportsFromText skipping uses:/path: keys: The filter correctly detects object-form YAML list items after the - prefix is stripped. New tests cover both forms and verify plain string imports in the same list are still extracted.
Bug 2 — resolveRemoteSymlinks startIndex optimization: The index math is correct for all path shapes:
.github/workflows/foo.md(3 parts, startIndex=3): loop never runs — no API calls for known non-symlink dirs..github/workflows/dir/foo.md(4 parts, startIndex=3): onlydiris probed — correct..github/agents/...paths still check.github/agents(startIndex=2).
The updated test assertion (callCounts.get(".github") → toBeUndefined) correctly reflects the new behaviour.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 27.4 AIC · ⌖ 4.33 AIC · ⊞ 5K
Skills-Based Review 🧠Applied 📋 Key Themes & HighlightsKey Themes
Positive Highlights
@copilot please address the review comments above.
|
There was a problem hiding this comment.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 23.4 AIC · ⌖ 4.63 AIC · ⊞ 6.7K
Comment /matt to run again
| const result = extractImportsFromText(frontmatterText); | ||
| expect(result).toEqual(["shared/test.md"]); | ||
| }); | ||
|
|
There was a problem hiding this comment.
[/tdd] Misleading test name — the test verifies that uses:-form items are skipped, not extracted.
💡 Suggested rename
it("should skip object-form uses: imports (requires full YAML parser)", () => {A test name should read as a specification. "should extract path from uses: format" implies the path is returned, but the assertion shows it is excluded. Renaming makes the intent clear and prevents a future contributor from "fixing" the test.
@copilot please address this.
There was a problem hiding this comment.
Fixed in 6e6a265. The test was renamed to "should extract path from object-form uses: import" and the assertion updated to expect(result).toEqual(["./serena.md", "shared/common.md"]) — uses: imports are now extracted (not skipped).
| // Skip YAML object-form imports (uses: <path> / path: <path>) — these require full | ||
| // YAML parsing and are not supported by the text-based hash parser. The Go implementation | ||
| // silently skips them at the filesystem level (file not found); we skip them here to | ||
| // match that behavior and avoid spurious GitHub API calls. |
There was a problem hiding this comment.
[/diagnosing-bugs] The uses: / path: skip silently drops object-form imports without surfacing the limitation anywhere. If a workflow heavily relies on uses:-form imports, the hash computation will silently under-count dependencies — potentially missing re-runs when those imports change.
💡 Suggestion
Emit a core.debug (or console.debug) log when an item is skipped so that diagnostics can confirm the skip is intentional rather than a silent data loss:
if (item.startsWith("uses:") || item.startsWith("path:")) {
// debug only; callers cannot act on it at hash-compute time
// console.debug(`[frontmatter_hash] skipping object-form import: ${item}`);
continue;
}Alternatively, document in a CONTEXT.md or inline comment that the hash may be conservative (always triggers a re-run) rather than exact when object-form imports are present, so future maintainers understand the known gap.
@copilot please address this.
There was a problem hiding this comment.
Addressed in 6e6a265 by fixing the root cause: extractImportsFromText now extracts the path from uses: and path: prefixes instead of skipping them. Object-form imports are fully tracked in the hash, so there is no longer a silent data loss to surface via a debug log.
| expect(result).toEqual(["shared/common.md"]); | ||
| }); | ||
|
|
||
| it("should extract path from path: format", () => { |
There was a problem hiding this comment.
[/tdd] Same misleading test name issue on the path: variant — the test name says "extract" but the behaviour is "skip".
Consider: "should skip object-form path: imports (requires full YAML parser)".
@copilot please address this.
There was a problem hiding this comment.
Fixed in 6e6a265. Renamed to "should extract path from object-form path: import" and assertion updated to expect(result).toEqual(["shared/tool.md"]) — path: imports are now extracted (not skipped).
There was a problem hiding this comment.
Requesting changes: Bug 1's fix is semantically incorrect and introduces a new hash divergence between the JS and Go implementations.
📋 Findings summary
Critical: JS/Go hash divergence for object-form imports
The fix to extractImportsFromText (line 279) silently skips uses: and path: object-form imports. But Go's import_topological.go (lines 227–234) extracts the path value and includes it in the hash. Any workflow containing - uses: ./foo.md will produce different hashes in JS vs Go, defeating the cross-language hash verification that this code exists to ensure.
The fix should extract the path value after the colon, not skip the entry entirely.
Medium: Test name is semantically wrong
The test at line 170 is named 'should extract path from uses: format' but asserts that the path is skipped. Minor but will confuse future readers and mask regressions.
No issues with Bug 2 fix
The resolveRemoteSymlinks startIndex logic (lines 152–157) is correct. The path-prefix conditions properly skip non-symlink .github prefix components.
🔎 Code quality review by PR Code Quality Reviewer · 68.1 AIC · ⌖ 4.67 AIC · ⊞ 5.6K
Comment /review to run again
Comments that could not be inline-anchored
actions/setup/js/frontmatter_hash_pure.cjs:279
Hash divergence: uses:/path: object-form imports are silently skipped in JS but extracted in Go, causing frontmatter hash mismatches for any workflow that uses object-form import syntax.
<details>
<summary>💡 Details and suggested fix</summary>
The Go parser in import_topological.go (lines 227–234) explicitly extracts the path value from object-form imports and appends it to the hash input:
case map[string]any:
if pathValue, hasPath := importItem["path"]; hasPath {
…
</details>
<details><summary>actions/setup/js/frontmatter_hash_pure.test.cjs:170</summary>
**Misleading test name**: `'should extract path from uses: format'` describes extraction, but the assertion verifies that the `uses:` item is *skipped*, not extracted.
<details>
<summary>💡 Details</summary>
The test asserts `expect(result).toEqual(["shared/common.md"])` — i.e., the `uses: ./serena.md` entry is absent from the result. But the test name says "extract path from uses: format", which implies the opposite behavior (that the path _is_ extracted and returned).
Beyond being confusin…
</details>
🤖 PR Triage
Rationale: Targeted JS hash-reader fix (2 bugs, 7 files, +57/-15). Copilot bot reviewed, github-actions bot APPROVED. CI in-progress but passing. Prevents silent invalid API requests in hash computation.
|
|
Hey
|
|
@copilot run pr-finisher skill |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
…ation - JS extractImportsFromText: extract path value from `uses:` and `path:` prefixes instead of skipping, so object-form imports are included in the frontmatter and body hashes. Changes to imported files now correctly invalidate the cached hash. - Go extractImportItem: same extraction fix using strings.CutPrefix for hash parity between JS and Go implementations. - Update JS tests: assertions and names updated to reflect extraction (not skip) of object-form import paths. - Add Go tests: TestExtractImportsFromText_ObjectFormUsesImport and TestExtractImportsFromText_ObjectFormPathImport. - Add JS regression test: asserts .github and .github/workflows are never probed by resolveRemoteSymlinks for a .github/workflows/ path. - Revert unintended schedule drift in agentic-auto-upgrade.yml (Friday → Saturday cron change was generated noise, not an intentional change). - Recompile all 257 workflow lock files to reflect updated hash values. Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
All review threads addressed in commit |
|
@copilot merge main and recompile |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
…13232204 # Conflicts: # .github/workflows/aw-failure-investigator.lock.yml # .github/workflows/code-scanning-fixer.lock.yml # .github/workflows/daily-function-namer.lock.yml # .github/workflows/daily-malicious-code-scan.lock.yml # .github/workflows/glossary-maintainer.lock.yml # .github/workflows/pr-code-quality-reviewer.lock.yml # .github/workflows/refiner.lock.yml # .github/workflows/security-review.lock.yml # .github/workflows/skillet.lock.yml # .github/workflows/stale-repo-identifier.lock.yml # .github/workflows/unbloat-docs.lock.yml Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Done. Merged |
|
🎉 This pull request is included in a new release. Release: |
Two bugs in the JS text-based hash computation caused
createGitHubFileReaderto make invalid GitHub API requests to paths like.githuband.github/workflows— paths that don't resolve to files and broke the invariant that all fetched paths must be under.github/workflows/.Bug 1:
extractImportsFromTextemittinguses:/path:keys as import pathsYAML object-form imports like:
…were extracted as the raw string
uses: ./serena.md. In JS,path.join("a/b/c", "uses: ./serena.md")preserves the embedded./as a path separator, producing extra segments. Go normalizes this away, so the mangled path hits a non-existent file and is silently discarded. Fix: skip items where the extracted value starts withuses:orpath:to match Go's effective behavior.Bug 2:
resolveRemoteSymlinksprobing too-shallow path prefixesNon-relative imports inside subdirectory workflows (e.g.
shared/otlp.mdfromshared/reporting-otlp.md) produce doubled-directory paths (shared/shared/otlp.md) viapath.join. The resulting 404 triggeredresolveRemoteSymlinksstarting at index 1, which probed.githubalone — a directory, not a workflow file. Fix: use a path-aware minimumstartIndex:.github/workflows/…→ start at 3 (skip.github+.github/workflows).github/…→ start at 2 (skip.github, preserving e.g..github/agentssymlink resolution)